Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Thread-safe signalling inside Mutex class. #99697

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Thread-safe signalling inside Mutex class. #99697

wants to merge 1 commit into from
+76 −12

Conversation

RadiantUwU
Copy link
Contributor

Recently i noticed an issue that doesn't allow you to await on a signal due to it being unsafe. This pull request's objective is to allow you to at least somehow make safe signals, even if we can't have them inside the normal classes, we can still guarantee thread-safety on some signals, right?

Overrides all signal behavior on Mutex, making sure it holds the mutex lock before doing anything.

@RadiantUwU RadiantUwU requested review from a team as code owners November 25, 2024 23:42
@clayjohn clayjohn added this to the 4.x milestone Nov 25, 2024
@RandomShaper
Copy link
Member

I think it won't come as a surprise to you that this looks quite awkward. While there is some merit in the idea of having some kind of opt-in for thread-safe signals, how to add such a thing to the engine would require deep discussion (normally, in a proposal) to be sure it aligns with the overall design of the engine and it actually covers the intended use patterns.

@AThousandShips
Copy link
Member

Could you please add some usage examples to show how this would be used to solve thread safety? I'm guessing you would use add_user_signal?

How would this solve the main thread safety concern with signals, namely that signals are disconnected on destruction of the listener, which seems to be the main cause of thread unsafety of signals and especially how it isn't solvable with external locking, the Object destructor doesn't call the exposed disconnect method for this, see:

godot/core/object/object.cpp

Lines 2150 to 2162 in d09d82d

// Disconnect signals that connect to this object.
while (connections.size()) {
Connection c = connections.front()->get();
Object *obj = c.callable.get_object();
bool disconnected = false;
if (likely(obj)) {
disconnected = c.signal.get_object()->_disconnect(c.signal.get_name(), c.callable, true);
}
if (unlikely(!disconnected)) {
// If the disconnect has failed, abandon the connection to avoid getting trapped in an infinite loop here.
connections.pop_front();
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
feature proposal topic:core
Projects
None yet
Milestone
4.x
Development

Successfully merging this pull request may close these issues.
4 participants
@RadiantUwU @RandomShaper @AThousandShips @clayjohn